-
Notifications
You must be signed in to change notification settings - Fork 446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client: improve implementation of CPU throttling #5233
Conversation
Old: if target CPU usage is (say) 1%, we'd unthrottle, sleep 1 sec, throttle, and sleep 99 sec. A change to the usage pref wouldn't be enforced during that sleep. New: do something every second; changes are seen immediately
The code looks fine to me. |
I'd say 'inconclusive'. Testing on Windows 7, I couldn't reproduce the stated problem with client 7.22.1: there didn't seem to be a delay when switching back from 1% to 100% of time. Having said that, it isn't obvious when CPU usage is paused - the task status in the Manager display (Advanced - tasks) remains as 'Running' throughout, and the only event logged is the 'Resume' (twice per task) when cpu_sched logging is enabled. The only clue is that elapsed time stops incrementing.
I switched to the artifact for this PR, with some difficulty: Windows Security Essentials flagged the client archive as a virus (Manager and apps were clean). I had to download it on a Linux machine and email it to myself ... I couldn't detect any change in behaviour between old and new. |
@RichardHaselgrove, thank you for testing this.
I have asked the reported of this issue to test this on their side.
Definitely a false positive detection. If you have any doubts - you can use this service to check any file with several endpoint security software: https://www.virustotal.com/gui/home/upload |
if (limit >= 100) { | ||
if (gstate.tasks_throttled) { | ||
gstate.active_tasks.unsuspend_all(SUSPEND_REASON_CPU_THROTTLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need the mutex locking?
} | ||
x -= 100; | ||
} else { | ||
if (!gstate.tasks_throttled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there still a problem that changing prefs triggers a reschedule, which will resume any suspended task without resetting tasks_throttled
, so we don’t get back in here even if we should?
which can run jobs that are currently throttled. Clear the tasks_throttled flag, so that the throttling logic can throttle these again if needed.
I think I fixed the problem Brian pointed out, and one or two others. Please test again. |
Old: if target CPU usage is (say) 1%,
we'd unthrottle, sleep 1 sec, throttle, and sleep 99 sec. A change to the usage pref wouldn't be enforced during that sleep.
New: do something every second; changes are seen immediately.
This makes a difference only when setting is very close to 0 or 100
Fixes #5231